Add API functions for the other kinds of matrixes that a CRN ODE system can be factored into #1134
Conversation
|
@isaacsas i think this is done mod the Documentation build thing |
src/network_analysis.jl
Outdated
|
|
||
| !isempty(pmap) && (rates = substitutevals(rn, pmap, parameters(rn), rates)) | ||
| rcmap = reactioncomplexmap(rn); nc = length(rcmap); nr = length(rates) | ||
| mtype = eltype(rates) <: Symbolics.BasicSymbolic ? Num : eltype(rates) |
There was a problem hiding this comment.
Should this be Num and not the unwrapped type?
There was a problem hiding this comment.
I think it has to, doesn't seem like it's actually possible to have zeros in this matrix if the eltype is BasicSymbolic
There was a problem hiding this comment.
Is that better than being Num? I assumed more specific was better
There was a problem hiding this comment.
We don't want to return mixtures of Nums and non-Nums across different methods, so we should not re-wrap internal symbolics.
There was a problem hiding this comment.
Could I just wrap it in an Any when I return it? It's useful to have it be Num at least temporarily because I use zero(T) and one(T) in various places (and product over an empty Num[] returns 1, which is useful).
There was a problem hiding this comment.
Actually this creates some issues for the sparse method (can't really do any arithmetic with a SparseMatrixCSC{Any, T} since zero(Any) is undefined). Would Union{Float64, BasicSymbolic} be okay? If not it might just be better to define two functions, one symbolic and one numeric
There was a problem hiding this comment.
If you want to internally use Num I guess that is ok, but yeah, we should unwrap before returning to users for consistency.
There was a problem hiding this comment.
What about returning a Union as the element type?
There was a problem hiding this comment.
Actually that doesn't work, nevermind, when you find the Laplacian matrix (D*K) type inference infers its eltype as Any anyway. Since it's the sparse matrix support that's causing the issue I think it might just be better to not allow the sparse kwarg for now
src/network_analysis.jl
Outdated
|
|
||
| !isempty(pmap) && (rates = substitutevals(rn, pmap, parameters(rn), rates)) | ||
| rcmap = reactioncomplexmap(rn); nc = length(rcmap); nr = length(rates) | ||
| mtype = eltype(rates) <: Symbolics.BasicSymbolic ? Num : eltype(rates) |
There was a problem hiding this comment.
We don't want to return mixtures of Nums and non-Nums across different methods, so we should not re-wrap internal symbolics.
|
It looks like right now tests are failing? |
Specifically
laplacianmat,fluxmat, andmassactionvector. Sorry about the terrible looking diff, I just split a lot of the more chemical reaction network-related tests (detailed/complex balance, deficiency theorems, concentration robustness) into a new file crn_theory.jl and added the tests for this to the bottom of thenetwork_analysis.jl. (Realize I should have done the file splitting in a separate PR but was in too deep already). Closes #1132.